Skip to content

fix: acceptance tests for secrets test output [PS-357]#6628

Open
alexandru-manea-snyk wants to merge 1 commit intomainfrom
fix/PS-357/extension-secrets-output-acceptance-tests
Open

fix: acceptance tests for secrets test output [PS-357]#6628
alexandru-manea-snyk wants to merge 1 commit intomainfrom
fix/PS-357/extension-secrets-output-acceptance-tests

Conversation

@alexandru-manea-snyk
Copy link
Contributor

@alexandru-manea-snyk alexandru-manea-snyk commented Mar 9, 2026 •
edited
Loading

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Commit messages
    are release-note ready, emphasizing
    what was changed, not how.
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)
  • Links to automated tests covering new functionality
  • Includes manual testing instructions (if necessary)
  • Updates relevant GitBook documentation (PR link: ___)
  • Includes product update to be announced in the next stable release notes

What does this PR do?

This PR adds acceptance tests to validate the SARIF and human-readable outputs of the secrets test command. These tests are designed to codify our rendering expectations and serve as a shared contract for the CLI team to iterate against.

Where should the reviewer start?

  • test/jest/acceptance/snyk-secrets/snyk-secrets-test-user-journey.spec.ts;
  • the document linked in the ticket;

How should this be manually tested?

Run the acceptance tests locally.

What's the product update that needs to be communicated to CLI users?

N/A

Risk assessment (Low | Medium | High)?

Low - extends test suite.

What are the relevant tickets?

@snyk-io
Copy link

snyk-io bot commented Mar 9, 2026 •
edited
Loading

✅ Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues
✅ Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch 6 times, most recently from 97767d5 to b04f5e4 Compare March 13, 2026 13:25
@alexandru-manea-snyk alexandru-manea-snyk marked this pull request as ready for review March 13, 2026 13:25
@alexandru-manea-snyk alexandru-manea-snyk requested review from a team as code owners March 13, 2026 13:25
@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch from b04f5e4 to 624110a Compare March 13, 2026 14:42
@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch from 624110a to 1352c55 Compare March 13, 2026 15:02
@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
CatalinSnyk
CatalinSnyk reviewed Mar 17, 2026
View reviewed changes
test/jest/acceptance/snyk-secrets/snyk-secrets-test-user-journey.spec.ts
const projectRoot = resolve(__dirname, '../../../..');

const TEST_REPO_COMMIT = '366ae0080cc67973619584080fc85734ba2658b2';
const TEST_REPO_URL = 'https://github.com/leaktk/fake-leaks';
Copy link
Contributor

@CatalinSnyk CatalinSnyk Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question/suggestions: Since this is an outside Snyk repository, did we consider vendoring it? We could also look to add it to the fixtures directory since we only scan a few of the available paths (but this might make gitleaks/GitGuardian unhappy, so we will need some ignores in place).

CatalinSnyk
CatalinSnyk reviewed Mar 17, 2026
View reviewed changes
test/jest/acceptance/snyk-secrets/snyk-secrets-test-user-journey.spec.ts
// Ignore the target issues
for (const [index, issueId] of issuesToIgnore.entries()) {
const reason = `Test ignore reason metadata ${index}`;
await runSnykCLI(
Copy link
Contributor

@CatalinSnyk CatalinSnyk Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Not sure if I missed this before, but I was expecting this to run the IAW flow, basically using the finding ID to do the ignores. I did also try it out and the .snyk file does not seem to work for ignoring the issues.

@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch from 1352c55 to 7f2d38b Compare March 18, 2026 08:48
@snyk-pr-review-bot
Copy link

snyk-pr-review-bot bot commented Mar 18, 2026

PR Reviewer Guide 🔍

🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Logic Error in Test Setup 🟠 [major]

The setupIsolatedIgnoreEnv function (lines 75-122) intended to prepare multiple unique ignores for testing, but it only copies a single source file (detected-sendgrid-api-key.txt) twice into the test directory (lines 96-98). Because both files have the same content, they trigger the same ruleId. Consequently, the JSON scan on line 101 only identifies one unique rule ID, causing the ignore loop (lines 109-115) to execute only once. This will lead to a failure in the test "should correctly render multiple ignores..." at line 301, which expects a count of 2 or more ignores (/Ignored:\s*[2-9]/), and at line 310, which expects metadata for "metadata 1" that is never created. To fix this, the setup should copy a second, different secret file to ensure at least two unique rule IDs are present.

 
const sourceFile = join(basePath, 'semgrep-rules-examples', 'detected-sendgrid-api-key.txt');
copyFileSync(sourceFile, join(testDir, `sendgrid-keys_1_${uuid}.txt`));
copyFileSync(sourceFile, join(testDir, `sendgrid-keys_2_${uuid}.txt`));
📚 Repository Context Analyzed

This review considered 8 relevant code sections from 7 files (average relevance: 0.91)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CatalinSnyk CatalinSnyk CatalinSnyk left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alexandru-manea-snyk @CatalinSnyk